Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify when Manager.from_queryset happens inside model class body #824

Merged

Conversation

flaeppe
Copy link
Member

@flaeppe flaeppe commented Jan 20, 2022

Whenever Manager.from_queryset happens inside of a model class body, an error will be displayed

e.g. Doing

class MyModel(models.Model):
    objects = BaseManager.from_queryset(MyQuerySet)()

Yields:

error: `.from_queryset` called from inside model class body

Note: I've also included a refactoring commit for config parsing, that hopefully will make it less of a hassle to introduce new settings in the future.

Related issues

Refs #738

@flaeppe flaeppe force-pushed the feature/manager-type-in-classbody-option branch from 05bba51 to 9959813 Compare January 20, 2022 20:43
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

README.md Outdated Show resolved Hide resolved
mypy_django_plugin/config.py Show resolved Hide resolved
- A warning will be emitted whenever `Manager.from_queryset` happens
  inside of a model class body
@flaeppe flaeppe force-pushed the feature/manager-type-in-classbody-option branch from 9959813 to 428c34d Compare January 21, 2022 13:12
@flaeppe flaeppe changed the title Option for notifying when Manager.from_queryset happens inside model class body Notify when Manager.from_queryset happens inside model class body Jan 21, 2022
@flaeppe
Copy link
Member Author

flaeppe commented Jan 21, 2022

I've removed the setting so that the new check is always enabled

@flaeppe
Copy link
Member Author

flaeppe commented Jan 21, 2022

Seems like some related manager issues popped up when enforcing the check. Gonna try to dig in and see if I can resolve them

@sobolevn
Copy link
Member

Feel free to ask any questions! 👍

Comment on lines +329 to +330
default_manager = related_model_info.get("_default_manager")
if not default_manager:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed from objects to _default_manager with https://docs.djangoproject.com/en/dev/topics/db/queries/#using-a-custom-reverse-manager as justification ("By default the RelatedManager used for reverse relations is a subclass of the default manager for that model.")

@@ -360,7 +358,7 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
def try_generate_related_manager(
self, related_model_cls: Type[Model], related_model_info: TypeInfo
) -> Optional[Instance]:
manager = related_model_cls._meta.managers_map["objects"]
manager = related_model_cls._meta.managers_map["_default_manager"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed from objects to _default_manager with https://docs.djangoproject.com/en/dev/topics/db/queries/#using-a-custom-reverse-manager as justification ("By default the RelatedManager used for reverse relations is a subclass of the default manager for that model.")

A default manager on a model should always exist, eventually. Although,
we extend to look through dynamically generated managers on each
iteration instead of deferring until the final iteration.
@flaeppe flaeppe force-pushed the feature/manager-type-in-classbody-option branch from 8fa5f52 to 2ee2995 Compare January 21, 2022 16:34
@flaeppe
Copy link
Member Author

flaeppe commented Jan 21, 2022

Alright, I think the related manager issues are resolved

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@sobolevn sobolevn merged commit edec5a1 into typeddjango:master Jan 21, 2022
@flaeppe flaeppe deleted the feature/manager-type-in-classbody-option branch January 21, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants